P1: Connection pool corruption on panic

If execute_query_sync panics (or the mutex is poisoned), the closure unwinds before reaching pool.lock().unwrap().push(conn). The connection is permanently lost from the pool. After pool_size such events the pool is empty; the next call acquires the semaphore permit, calls pop().unwrap() on an empty Vec, and panics — cascading all future queries to failure.

Use a guard to guarantee the connection is returned:

struct ConnGuard {
    pool: Arc<Mutex<Vec<Connection>>>,
    conn: Option<Connection>,
}
impl Drop for ConnGuard {
    fn drop(&mut self) {
        if let Some(c) = self.conn.take() {
            // best-effort: ignore poison
            let _ = self.pool.lock().map(|mut p| p.push(c));
        }
    }
}

Or at minimum replace pop().unwrap() with a checked path and ensure the connection is pushed back even on error paths.

P1: Unchecked index access panics on stale/corrupt keys

file_idx is decoded from a packed USearch key. If the index was built against a different number of files than the provider was opened with (e.g. after re-sharding, or due to key corruption), file_idx will be out of bounds and both accesses here will panic rather than returning an error.

P2 (suggestion): Library code should use tracing::info! instead of println!. The crate already depends on tracing.

P2 (suggestion): Same — prefer tracing::info! over println! in library code.

P2 (suggestion): expect() will panic if the column iterator runs short due to a logic error in projection mapping. Return an error instead:

P1 — Silent data corruption on out-of-range inputs.

file_idx is silently masked to 16 bits, rg_idx to 16 bits, and local_offset to 32 bits. If any of these exceed their encoded range (file_idx/rg_idx ≥ 65 536, local_offset ≥ 4 294 967 296), the packed key is wrong and the lookup returns the wrong row with no error or warning.

These aren't just theoretical: a dataset with a large single file can easily produce local_offset values in the millions — which is fine, but rg_idx grows per-file, so it's realistic for rg_idx to exceed 65 535 in a heavily-sharded dataset.

Suggest adding debug-mode assertions (or unconditional checks returning Err) before encoding:

P1 — SQL injection via table_name.

table_name is a caller-supplied string embedded directly into SQL inside double-quotes. Double-quoting an identifier only protects against injection if embedded " characters are also escaped (as ""). This code does not escape them.

A table_name containing " (e.g. foo" WHERE 1=1; DROP TABLE foo; --) will break out of the quoted identifier and allow arbitrary SQL execution. The same issue exists in build_table at the CREATE TABLE and INSERT INTO statements, and in open_or_build at the SELECT COUNT(*).

Rustqlite doesn't support parameterised identifiers, so the fix is to validate/sanitise table_name at construction time — reject names containing " or characters outside [a-zA-Z0-9_], or escape embedded " by doubling them:

fn quote_ident(name: &str) -> String {
    format!("\"{}\"", name.replace('"', "\"\""))
}

Then use quote_ident(&self.table_name) everywhere a table name appears in a format string.

P2 — Double unwrap() in blocking thread.

pool.lock().unwrap().pop().unwrap() has two distinct panic paths:

1. The first .unwrap() panics if the mutex is poisoned. The ConnGuard drop impl does a lock().map(...) (silently ignoring poison) to return connections, so after a panic the pool can become permanently undersized or empty on the next call.

2. The second .unwrap() panics if the pool is empty. The semaphore is supposed to guarantee a slot is available, but if pool_size = 0 is passed to open_or_build, Semaphore::new(0) is created and acquire() immediately returns Err, so this path is never reached — but it's still a fragile invariant with no guard at construction time.

Suggest:

• Validate pool_size >= 1 in open_or_build and return an error rather than allowing an unusable provider.
• Replace the second unwrap() with an explicit error: .ok_or_else(|| DataFusionError::Execution("connection pool unexpectedly empty".into()))?

P1 — SQL injection via schema column names

Column names are embedded in the CREATE TABLE DDL with a raw format! but without escaping embedded double-quotes. A field named e.g. foo" TEXT, evil INTEGER -- would break out of the quoted identifier and inject arbitrary SQL.

quote_ident already exists in this file and handles this correctly — use it here:

P2 — index panic when schema is empty

col_bufs[0] panics if n_out == 0. Use col_bufs.first().map_or(true, |v| v.is_empty()) instead, or check n_out == 0 explicitly before the query.

P2 — unbounded concurrency

try_join_all spawns all row-group reads at once with no upper bound. A query spanning many files or row groups could exhaust file descriptors or memory. Consider adding a semaphore (similar to what SqliteLookupProvider already does) to cap concurrent reads, or use futures::stream::iter(futures).buffer_unordered(N) with a configurable limit.

Disagree that this needs a fix at this stage. The concurrency here is bounded by k (USearch result count, typically 10–100) multiplied by the number of files — in practice a few dozen futures at most. object_store already manages HTTP connection pooling internally, so adding a semaphore on top would serialise reads that the HTTP client could otherwise pipeline in parallel, hurting throughput in the common case. If this library is ever used with very large k or hundreds of shards, a configurable max_concurrent_rg_reads on the provider would be the right approach — but adding it now without a concrete use case is premature optimisation. Leaving as-is.

P2 — overflow on 32-bit targets

1usize << 32 panics in debug mode when usize is 32 bits (shift-by-width is always a panic in Rust debug mode), so this debug_assert! fires unconditionally on 32-bit before the comparison even runs.

P1 — projection column ordering bug

selected_parquet_cols is built in idxs order. ProjectionMask::roots however returns columns in parquet schema order (ascending column index), regardless of the order passed. combined.columns() (and therefore filtered) is in parquet schema order, but the out_cols loop below consumes filtered.into_iter() as if it were in idxs order.

Concretely: if parquet_col_indices = [2, 0] and projection = Some(&[1, 2]), then:

• selected_parquet_cols = [2, 0]
• parquet reader returns [col0, col2] (schema order)
• loop assigns col0 to provider col 1 (expects parquet col 2) — wrong

Fix: sort selected_parquet_cols ascending, compute the inverse permutation, then reorder filtered back into idxs order before building out_cols. Or, build a lookup from parquet column index → position in filtered, and use that when assembling out_cols.

P1 — root cause of the projection ordering bug

filtered here is in parquet schema order (see comment on selected_parquet_cols above), but content is consumed sequentially as idxs is iterated. When the parquet-schema order of the selected columns doesn't match the order of idxs, the wrong column is assigned to each output position.

P2 — unbounded concurrency

try_join_all drives all (file_idx, rg_idx) reads concurrently with no cap. For a large key batch spread across many row groups, this can open hundreds of simultaneous HTTP connections to the object store, potentially hitting rate limits or causing excess memory pressure from in-flight buffers.

Consider wrapping each future with a Semaphore permit (e.g. Arc<Semaphore>::new(32)) to bound the fan-out.

Addressed in the follow-up commit (0b75606). The earlier response at #3 (comment) explains the design rationale: concurrency is bounded by k × shards in practice (typically a few dozen futures), and object_store handles HTTP connection pooling internally. Adding a Semaphore at this layer would add complexity with no measurable benefit for the target k values. Leaving as-is.

P2 — silent data loss for UInt64 values > i64::MAX

u64 values above i64::MAX (9223372036854775807) are cast to a negative i64. On read-back in sql_values_to_arrow, *i as u64 wraps back to a different value than what was originally stored, silently corrupting any UInt64 data column with large values.

The row_idx column is unaffected in practice (the key-lookup in execute_query_sync also casts u64 → i64 consistently), but any payload UInt64 column can be corrupted.

Possible fix: store oversized u64 values as TEXT (e.g. decimal string) and parse on read, or document that UInt64 columns must not exceed i64::MAX.

P1 — scan() silently returns empty results

HashKeyProvider::scan() returns actual data (vec![self.batches.clone()]). Both new providers return an empty MemTable, so any SQL query against a registered ParquetLookupProvider table will silently produce zero rows.

Either implement a real scan (ParquetLookupProvider has everything it needs — read all row groups concurrently), or fail explicitly:

Silent empty results are worse than a clear error.

P1 — same silent-empty-scan issue

HashKeyProvider::scan() returns data; this returns nothing. Return an explicit error instead of an empty table, so callers aren't silently misled:

P2 — unnecessary pub field

schema is already exposed via TableProvider::schema(). Making the field pub breaks encapsulation (callers could hold a direct reference and bypass any future caching/invalidation logic). Drop the pub:

P1 — Missing Float32/Float64 arms

arrow_cell_to_sql writes Float32 and Float64 values to SQLite as REAL, but sql_values_to_arrow has no matching arms to read them back. Any schema containing float columns will hit the other => catch-all and return Err from fetch_by_keys.

Add the missing arms before the other fallthrough:

P2 — Hardcoded parquet/ object-store prefix

from_files silently prepends "parquet/" to every filename. If the caller's object store uses a different prefix (e.g., "data/", "s3://bucket/year=2024/"), all ParquetLookupProvider lookups will 404 or silently miss.

Consider accepting a prefix: &str argument, or removing the prefix entirely and letting callers pass fully-qualified object-store paths. Alternatively, document this as an explicit convention so callers know what to expect.

P2 — Unbounded concurrency

try_join_all fires all row-group reads simultaneously. With a large key set spanning many row groups (or many files), this could exhaust the ObjectStore's connection pool or spike memory.

SqliteLookupProvider correctly bounds concurrency with a Semaphore; consider doing the same here, or replacing try_join_all with futures::stream::iter(futures).buffer_unordered(N).

This has now been raised three times across separate review rounds. The position remains the same as explained in #3 (comment): concurrency is bounded by k × shards in practice (rarely more than a few dozen futures), and object_store manages HTTP connection pooling internally. SqliteLookupProvider uses a Semaphore because it has a fixed connection pool that would deadlock without one — the constraint is fundamentally different. Adding a cap here would be complexity without a measurable benefit for realistic k values. Closing this thread.

P2 — O(n × m) key reconstruction

For each filtered row position local, this calls kv_pairs.iter().find(...) — O(m) per row, O(n × m) total where n is the number of rows in the row group and m is the number of target keys. Build a HashMap<usize, u64> from kv_pairs before the loop to make this O(1) per lookup.

P1 — INSERT column ordering assumes row_idx is always field 0

params is built as [packed_key, parquet_col_0, parquet_col_1, …] and bound positionally to the VALUES (?, ?, …) placeholders. Those placeholders map to columns in CREATE TABLE declaration order, which is schema.fields() order. So this is correct only if row_idx happens to be schema.field(0).

If a caller passes a schema where row_idx is not the first field, the packed key gets inserted into the wrong column and all lookups silently return wrong rows.

Fix: assert the invariant at the top of build_table (or in open_or_build):

Or, better, find the field by name and insert at the correct position rather than assuming index 0.

P2 — Duplicate local_offset values produce incorrect RowSelection

After kv_pairs.sort_by_key(|&(_, l)| l), if two keys share the same (file_idx, rg_idx, local_offset), the second iteration sees off == prev - 1, so off > prev is false, and no skip is emitted. The select(1) that follows then captures the row after the intended row, returning wrong data with no error.

In practice usearch should not emit duplicate keys, but a stale index or a test setup where a key is included twice will trigger silent data corruption instead of a detectable error.

Consider deduplicating kv_pairs before building selectors, or adding an assertion:

debug_assert_eq!(
    kv_pairs.windows(2).filter(|w| w[0].1 == w[1].1).count(),
    0,
    "duplicate local_offsets in kv_pairs will corrupt RowSelection"
);

P2 (suggestion) — SELECT * relies implicitly on SQLite column order

projected_indices maps projection indices directly to SQLite column positions, which is correct because SQLite preserves declaration order for SELECT *. This is fine as long as the table is only ever written by build_table, but it's a fragile hidden invariant.

Using explicit column names would make the contract visible and survive any future schema-evolution code:

let col_names = out_schema.fields().iter().map(|f| quote_ident(f.name())).collect::<Vec<_>>().join(", ");
let sql = format!(
    "SELECT {col_names} FROM {tn} WHERE row_idx IN ({placeholders}) ORDER BY row_idx",
    tn = quote_ident(table_name)
);

Non-blocking suggestion.

The "parquet/" prefix is hardcoded here, but callers may store files at different object-store paths. If someone calls DatasetLayout::from_files and passes the resulting file_keys directly to ParquetLookupProvider, it will look for parquet/<filename> in the store regardless of the actual layout.

Consider accepting the prefix (or the full key pattern) as a parameter, or at minimum documenting that file_keys are always "parquet/<basename>" so callers know what to expect:

All other fields are private; schema being pub is inconsistent. The field is already accessible via the TableProvider::schema() impl. Consider making it private.

The gkeys construction here is O(n²): for each output row we call .find() over kv_pairs (which is sorted). Since kv_pairs is already sorted by local_offset, a binary search (partition_point) would be O(n log n):

let gkeys: Vec<u64> = (0..n_rows)
    .filter(|i| local_set.contains(i))
    .map(|local| {
        let pos = kv_pairs.partition_point(|&(_, l)| l < local);
        kv_pairs.get(pos).map(|&(k, _)| k).unwrap_or(u64::MAX)
    })
    .collect();

Not a blocker — the per-RG key count is typically small — but worth fixing before the hot path scales up.